Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to testing.RaisesGroup for catching unwrapped exceptions #2989

Merged
merged 30 commits into from
May 17, 2024

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Apr 15, 2024

with RaisesGroup(ValueError, TypeError, strict=False):
  raise ExceptionGroup("", [ExceptionGroup("", [ValueError(), TypeError()])])

would fail due to it checking if the number of exceptions was correct before flattening the structure of the raised exceptiongroup.

I also found an unrelated issue where mypy&pyright are not able to deduce the type when passing multiple Matcher objects matching against different exceptions to RaisesGroup. (or Matcher+class, but two classes work). Fiddled around a little bit but wasn't able to quickly find a fix, and explicitly setting [Exception] isn't especially onerous.
EDIT: this was due to TypeVar lacking covariance.

Copy link

codecov bot commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.63%. Comparing base (ccd40e1) to head (6ef442b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2989   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files         120      120           
  Lines       17710    17798   +88     
  Branches     3179     3198   +19     
=======================================
+ Hits        17645    17733   +88     
  Misses         46       46           
  Partials       19       19           
Files Coverage Δ
src/trio/_core/_run.py 99.34% <ø> (ø)
src/trio/_core/_unbounded_queue.py 100.00% <ø> (ø)
src/trio/_deprecate.py 100.00% <100.00%> (ø)
src/trio/_highlevel_open_tcp_listeners.py 100.00% <ø> (ø)
src/trio/_tests/test_deprecate.py 100.00% <100.00%> (ø)
src/trio/_tests/test_testing_raisesgroup.py 100.00% <100.00%> (ø)
src/trio/_threads.py 100.00% <ø> (ø)
src/trio/testing/_raises_group.py 100.00% <100.00%> (ø)

@jakkdl
Copy link
Member Author

jakkdl commented Apr 15, 2024

Oh, turns out the unrelated typing error does work with pyright even with the overloads, and it's solely a mypy thing. I'll try to get a minimal repro and/or see if it's a known mypy issue and/or report it.
EDIT: fixed by making TypeVar covariant

src/trio/_tests/type_tests/raisesgroup.py Outdated Show resolved Hide resolved
src/trio/testing/_raises_group.py Outdated Show resolved Hide resolved
src/trio/testing/_raises_group.py Outdated Show resolved Hide resolved
@jakkdl
Copy link
Member Author

jakkdl commented Apr 16, 2024

@TeamSpen210 after adding covariant=True to the TypeVar the docs/source/typevars.py fails to resolve it:

/home/h/Git/trio/looser_excgroups/src/trio/testing/__init__.py:docstring of trio.testing.RaisesGroup:1: WARNING: py:obj reference target not found: typing.Callable[[BaseExceptionGroup[+E]], bool] | None
/home/h/Git/trio/looser_excgroups/src/trio/testing/__init__.py:docstring of trio.testing.Matcher:1: WARNING: py:class reference target not found: type[+E] | None
/home/h/Git/trio/looser_excgroups/src/trio/testing/_raises_group.py:docstring of trio.testing._raises_group._ExceptionInfo.type:1: WARNING: py:class reference target not found: type[+E]

I'm a bit surprised as covariant typevars exist in a few other places in the codebase without issue. Unless you have any opinions I'll try do some ugly workaround where I either add a plussed [copy] to the dicts in identify_typevars, or strip +s from the target in lookup_reference

EDIT: nope, wasn't that easy to work around...

@jakkdl
Copy link
Member Author

jakkdl commented Apr 16, 2024

I can make a theoretical case for splitting strict=False into two separate flags - one that allows unwrapped exceptions, and one that flattens nested exceptiongroups. But I suspect the added burden of having to set both might outweigh the gain from somebody that actually wants them split:

# they want this to pass
with RaisesGroup(ValueError, strict=False):
    raise ExceptionGroup("outer", [ExceptionGroup("inner", [ValueError()])])
# but this to fail
with RaisesGroup(ValueError, strict=False):
    raise ValueError

Of course they can just do

with RaisesGroup(..., strict=False) as e:
    ...
assert isinstance(e, ExceptionGroup)

but that's easy to forget and could silently introduce changes in behavior.

One, probably better, case in favor of splitting out allow_unwrapped=True is that we can raise an error in RaisesGroup.__init__ if specifying several exceptions + allow_unwrapped=True. That's both easier to implement and faster to debug for end-users than trying to resolve that with a message on a failed catch #2989 (comment)

Since typing with RaisesGroup(ValueError, allow_unwrapped=True, flatten=True) is a mouthful we (or leave that for pytest) could add convenience aliases, or just rely on end-users to write their own thin wrappers that change the defaults.

@jakkdl jakkdl requested a review from Zac-HD April 16, 2024 14:30
@TeamSpen210
Copy link
Contributor

Fixed the typevar-related issues, but now there's something else in history.rst. Not fully sure why the +E is only happening with this typevar, but I found a workaround. By using autodoc_process_signatures() I could insert the fully qualified name for the var, allowing Sphinx to find it.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, once the build passes let's merge!

src/trio/_tests/test_testing_raisesgroup.py Outdated Show resolved Hide resolved
newsfragments/2989.feature.rst Outdated Show resolved Hide resolved
jakkdl and others added 3 commits April 18, 2024 12:37
Co-authored-by: Zac Hatfield-Dodds <[email protected]>
…g where length check would fail incorrectly sometimes if using flatten_subgroups
@jakkdl
Copy link
Member Author

jakkdl commented Apr 18, 2024

Seems like nobody else had opinions on strict vs allow_unwrapped+flatten_subgroups. The ability to raise a ValueError on users doing RaisesGroup(SyntaxError, TypeError, allow_unwrapped=True) with a helpful message on what else to do convinced me that this is the way to go, even if it will be more verbose when you want to fully emulate except* (i.e. you expect a single exception, but don't care if it's unwrapped, or in any level of nesting).

@jakkdl jakkdl requested a review from Zac-HD April 18, 2024 13:31
@jakkdl jakkdl changed the title Add support to testing.RaisesGroup for catching unwrapped exceptions with strict=False Add support to testing.RaisesGroup for catching unwrapped exceptions Apr 18, 2024
newsfragments/2989.bugfix.rst Outdated Show resolved Hide resolved
newsfragments/2989.feature.rst Outdated Show resolved Hide resolved
newsfragments/2989.bugfix.rst Outdated Show resolved Hide resolved
newsfragments/2989.feature.rst Outdated Show resolved Hide resolved
Copy link
Member

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I think I saw a few regexes reading through the changes that could still have end specifiers added, but I think it should be ok.

@jakkdl
Copy link
Member Author

jakkdl commented Apr 24, 2024

Looks good. I think I saw a few regexes reading through the changes that could still have end specifiers added, but I think it should be ok.

The missing end specifiers are intended because I didn't bother including the full error message.

I fixed it so RaisesGroup now supports it on ExceptionGroup - previously it failed since it included "1 sub-exception" in the message it matched against, since

>>> str(ExceptionGroup('foo', [ValueError()]))
'foo (1 sub-exception)'

src/trio/_tests/test_testing_raisesgroup.py Outdated Show resolved Hide resolved
Comment on lines 345 to 351
warnings.warn(
DeprecationWarning(
"`strict=False` has been replaced with `flatten_subgroups=True`"
" with the introduction of `allow_unwrapped` as a parameter."
),
stacklevel=2,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't we using our deprecation utils in this case? Cause those would have the version this was deprecated in which is useful information.

Copy link
Contributor

@A5rocks A5rocks Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If it's because they don't warn with something derived from DeprecationWarning that makes sense. Maybe add a kwarg to warn_deprecated?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly just me being confused about the existence of TrioDeprecationWarning (hence #2992). One weird part with warn_deprecated is you have to know what the next version bump will be, but I'm guessing I should mark it as 0.25.1.
But I also don't think this one is worthy of an official extended period of supporting it, it's a trivial change to use flatten_subgroups=True instead and this is explicitly a temporary helper until it's merged into pytest, so it's barely worth having a DeprecationWarning at all. It's not used at runtime either, so it can only break tests. But I guess TrioDeprecationWarning doesn't have an explicit promise of how long it will be kept deprecated anyhow

@jakkdl jakkdl requested a review from A5rocks May 1, 2024 09:45
Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments. Sorry for taking a while, I wasn't sure whether we should merge this yet in case we want a patch release w/ those embedded Python fixes, but I haven't made that release yet so... Oh well.

src/trio/_tests/test_deprecate.py Show resolved Hide resolved
src/trio/_tests/test_testing_raisesgroup.py Show resolved Hide resolved
src/trio/_tests/test_testing_raisesgroup.py Show resolved Hide resolved
src/trio/testing/_raises_group.py Show resolved Hide resolved
@jakkdl jakkdl requested a review from A5rocks May 14, 2024 14:32
@jakkdl
Copy link
Member Author

jakkdl commented May 14, 2024

oh sorry for the re-request, didn't get notified about your review

@jakkdl
Copy link
Member Author

jakkdl commented May 14, 2024

oh sorry for the re-request, didn't get notified about your review

fixed everything now though, so re-review request is warranted

@jakkdl
Copy link
Member Author

jakkdl commented May 16, 2024

I encountered some issues with typing for functions passed to the check argument, and thought about handling that in a separate pull request - but it got so thorny that I'll just add the failing tests now and maaaybe address it in a separate PR once I hear from python/mypy#17251

Comment on lines +334 to +335
match: None = None,
check: None = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
match: None = None,
check: None = None,

I don't think it's necessary to provide these. a) we're adding the param now so no backcompat issues and b) I don't think we should care about backcompat for types because they're just hints.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think it doesn't hurt to have them, and allows users to do some automated stuff where they pass variables that they know to be None:

for allow_unwrapped,match in zip([True, None], [False, "hello"]):
  with RaisesGroup(Exception, allow_unwrapped=allow_unwrapped, match=match):
    ...

@A5rocks
Copy link
Contributor

A5rocks commented May 17, 2024

Also re: your mypy bug, my impression was that it just ignores __new__ in favor of __init__ if it's available.

@jakkdl jakkdl merged commit 6f62575 into python-trio:master May 17, 2024
28 checks passed
@jakkdl
Copy link
Member Author

jakkdl commented May 17, 2024

Also re: your mypy bug, my impression was that it just ignores __new__ in favor of __init__ if it's available.

scrolling through https://github.com/python/mypy/blob/master/test-data/unit/check-classes.test I guess that is the current state of things with mypy. But after sleeping on it I think I have a better way of resolving it - but that's for a new PR. It's finally time to merge this one 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants